-
Notifications
You must be signed in to change notification settings - Fork 197
FEAT: add dir as property #6716
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6716 +/- ##
==========================================
+ Coverage 83.23% 83.25% +0.02%
==========================================
Files 245 246 +1
Lines 77361 77491 +130
==========================================
+ Hits 64389 64519 +130
Misses 12972 12972 🚀 New features to boost your workflow:
|
# Conflicts: # src/ansys/aedt/core/generic/numbers.py
# Conflicts: # tests/system/general/test_01_general_methods.py
After doing some research, I see no objection to your proposal, as I could not find any resource discouraging this type of implementation. I think using the builtin I do understand the usecase for that new property and the usability improvement. Same goes for the All in all, the changes do look good to me (nice that tests are provided as well)! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening that PR @Alberto-DM
While I do like the idea of having some mixin that could share behavior over all classes, I'm not really fan of the dir
property. I might be wrong, but I feel like this property brings:
- risk of confusion for people who are familiar with
dir()
, this "breaks" implicit convention of the language; - little additional value when the risk of confusion are non negligible.
Also, I think we should focus on improving the type hints provided through the code base. This would really help a lot with autocompletion in modern IDEs. When thinking about notebook, isn't the use of already enough to get the attributes ? If it's because the list starts with private / protected attributes and methods, we could just rewrite__dir__
by sorting the elements and placing the one with_
at the end of the list.
Other though: renaming into public_attrs
would avoid confusion.
Open to discussion :)
@SMoraisAnsys I understand your concerns. Regarding the utility of having dir as a property: I personally use I also really like the idea of improving autocompletion by reordering the @ecoussoux-ansys yes, implementing |
# Conflicts: # src/ansys/aedt/core/visualization/post/post_circuit.py # src/ansys/aedt/core/visualization/post/solution_data.py
Following the suggestions from @SMoraisAnsys I renamed the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Alberto-DM We already have a file to hold Mixins (src/ansys/aedt/core/mixins.py) is there a reason why you didn't add your changes in that file ?
If the mixin used by the base class is imported from |
For visibility: following the comment above and a direct discussion, we agreed that the base class will be in it's own file and shouldn't have any dependency since it'll be used everywhere in the project. This will ensure that future changes do not conflict and cause circular import. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Samuelopez-ansys @maxcapodi78 please have a look at #6716 (comment)
Description
This PR introduces a
DirMixin
and a centralPyAedtBase
class to simplify interactive exploration and provide a unified foundation for all project classes.DirMixin
adds a.public_dir
property for instances to quickly inspect available attributes and methods, filtering out private/internal names.DirMixin
redefines the__dir__
method to reorder the output in order to improve IDEs autocompletion. The public attributes are shown first, followed by the private ones (starting with_
).PyAedtBase
inherits fromDirMixin
and serves as a central base class for all other classes, making it easy to add additional mixins or shared functionality in the future.Example
Prior to this feature the method exploration required to type back and forth in the command line:
Now we can explore the methods and properties of an object by simply doing this:
Benefits